-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid false unreachable
and redundant-expr
warnings in loops.
#18433
base: master
Are you sure you want to change the base?
Conversation
Fixes python#18348 Fixes python#13973 Fixes python#11612 Fixes python#8721 Fixes python#8865 Fixes python#7204 I manually checked all the listed issues. Some of them were already partly fixed by python#18180.
This comment has been minimized.
This comment has been minimized.
Such a long primer diff. It seems like I enabled |
Diff from mypy_primer, showing the effect of this PR on open source code: bandersnatch (https://github.com/pypa/bandersnatch)
+ src/bandersnatch_storage_plugins/s3.py:62: error: Unused "type: ignore" comment [unused-ignore]
core (https://github.com/home-assistant/core)
+ homeassistant/components/recorder/migration.py:2283: error: Unused "type: ignore" comment [unused-ignore]
+ homeassistant/components/recorder/migration.py:2305: error: Unused "type: ignore" comment [unused-ignore]
+ homeassistant/components/schedule/__init__.py:76: error: Unused "type: ignore" comment [unused-ignore]
+ homeassistant/components/twentemilieu/calendar.py:73: error: Unused "type: ignore" comment [unused-ignore]
pyodide (https://github.com/pyodide/pyodide)
+ src/py/pyodide/_state.py:21: error: Incompatible types in assignment (expression has type "pyodide._state.<subclass of "ModuleType" and "JsProxy">2", target has type "pyodide._state.<subclass of "ModuleType" and "JsProxy">") [assignment]
|
Regarding pypa/bandersnatch and home-assistant/core, the diff looks good. Regarding pyodide/pyodide, I neither understand why this error is emitted now nor why it is emitted in general (confusing error message). Maybe you know this behaviour already. I could investigate later. |
I can reproduce the pyodide problem with Mypy 1.14.1 if I repeat the class A: ...
class B: ...
a: A
if isinstance(a, B):
c = a
if isinstance(a, B):
c = a # E: Incompatible types in assignment (expression has type "__main__.<subclass of "A" and "B">1", variable has type "__main__.<subclass of "A" and "B">") [assignment] |
It's sort-of caused by the change here: at the final iteration (when partials don't change) you I don't believe this is a big deal though (various issues arise with building dicts/lists/... in a non-trivial loop, it's often necessary to annotate the collection anyway). |
I do not entirely understand what you are trying to say. However, the same problem can already occur with earlier Mypy versions (that do not check for the number of partials like the master branch Mypy does now) when writing a loop like this: class A: ...
class B: ...
a: A
cs = []
x: int | str
x = 1
for i in range(2):
if isinstance(a, B):
cs.append(a) # E: Incompatible types in assignment (expression has type "__main__.<subclass of "A" and "B">1", variable has type "__main__.<subclass of "A" and "B">") [assignment]
x = "x" Here, So, in my opinion, this issue does not need to bother us in this PR. However, as related false positives would likely be reported more often (in case this PR gets accepted) and as I am curious about the underlying cause, I could try to fix it in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG. Regarding the isinstance()
regression: why did we start indexing the ad-hoc intersections in the first place? IMO they all should be mutually compatible.
while True: | ||
with self.binder.frame_context(can_skip=True, break_frame=2, continue_frame=1): | ||
self.accept(body) | ||
partials_new = sum(len(pts.map) for pts in self.partial_types) | ||
if (partials_new == partials_old) and not self.binder.last_pop_changed: | ||
break | ||
partials_old = partials_new | ||
|
||
# If necessary, reset the modified options and make up for the postponed error checks: | ||
if warn_unreachable or warn_redundant: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this if
and the two if
s above are only making the logic more obscure. It should be simply: store, set, restore.
I couldn't track when we started distinguishing ad-hoc intersections, it looks like it is there since long time ago. Anyway, IMO instead of using |
Fixes #18348
Fixes #13973
Fixes #11612
Fixes #8721
Fixes #8865
Fixes #7204
I manually checked all the listed issues. Some of them were already partly fixed by #18180.